Skip to content

REST API: Custom cookie nonce authenticator #8659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 17 commits into from
Jan 18, 2023
Merged

REST API: Custom cookie nonce authenticator #8659

merged 17 commits into from
Jan 18, 2023

Conversation

@itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Jan 17, 2023 •
edited
Loading

Part of #8453

Description

There has been an ongoing issue with handling cookie-nonce authentication for Pressable sites (also mentioned by @joshheald in #8587). This blocks testing for these sites and can be an issue on production as well (though not reproducible or reported yet).

Previously we were using WordPressKit's CookieNonceAuthenticator to handle the authentication, but there's a potential issue with using the cookie nonce retrieval URL as the redirect URL for the login request.

This PR creates an updated authenticator that handles nonce retrieval as a separate request. This seems to fix the failures on Pressable sites. I'm not creating a PR to WordPressKit to avoid affecting other apps that are using this library.

Also: in order to avoid confusion, this PR also renames the existing RequestAuthenticator, RequestProcessor with the ApplicationPassword prefix.

Due to time constraints, unit tests for the authenticator will be added in a future PR.

Testing instructions

Pre-requisite: Make sure that you have a test site hosted on Pressable.

  • Enable the feature flag applicationPasswordAuthenticationForSiteCredentialLogin and build the app.
  • Log out of the app or skip onboarding if needed.
  • On the prologue screen, select Enter your site address and proceed with the address of your test site.
  • Log in with your site credentials. Notice that the login succeeds.
  • Please feel free to try a few times again to make sure that the authentication succeeds in case the failure is not consistent.

Screenshots

N/A

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@itsmeichigo itsmeichigo added type: enhancement A request for an enhancement. feature: REST API Authenticating requests using application password and using REST API instead of Jetpack tunnel. labels Jan 17, 2023
@itsmeichigo itsmeichigo added this to the 12.0 milestone Jan 17, 2023
@itsmeichigo itsmeichigo marked this pull request as ready for review January 17, 2023 04:53
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 17, 2023 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8659-d820410 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@jaclync jaclync self-assigned this Jan 18, 2023
jaclync
jaclync approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Login works with multiple Pressable sites! had some clarifying questions, feel free to merge to unblock testing other PRs

Networking/Networking/ApplicationPassword/ApplicationPasswordRequestProcessor.swift
/// Authenticates and retries requests
///
final class RequestProcessor {
final class ApplicationPasswordRequestProcessor {
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @selanthiraiyan can confirm the renaming of these classes since you created them? I didn't know they're only used in the application password context when I reviewed the previous PR on these classes.

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The processor triggers generating application passwords when retrying requests, so it is specific to application password authentication only. I renamed this to avoid confusion with any other authentication method.

Copy link
Contributor

@selanthiraiyan selanthiraiyan Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid that using ApplicationPassword as a prefix wouldn't make sense here.

The RequestProcessor handles authentication for both WPCOM token and application password cases. RequestProcessor uses DefaultRequestAuthenticator to perform the authentication here

Extra info: Only REST requests are retried, and errors from other requests are just passed through. Due to this method in AlamofireNetwork

Networking/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift Outdated

/// RequestProcessor Unit Tests
///
final class RequestProcessorTests: XCTestCase {
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we want to rename the test case name & file name as well?

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, updated in d820410.

Networking/Networking/CookieNonce/CookieNonceAuthenticator.swift Outdated
Comment on lines 10 to 12
// If we can't get this to work once, don't retry for the same site
// It is likely that there is something preventing us from extracting a nonce
private var canRetry = true
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ the comment seems to contradict the code canRetry = true? 🤔 my interpretation of the comment is that we're not retrying.

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is meant to say that we should allow retrying only once, but I've removed the comment since it can be misleading.

Networking/Networking/CookieNonce/CookieNonceAuthenticator.swift Outdated
// MARK: Request Adapter

func adapt(_ urlRequest: URLRequest) throws -> URLRequest {
guard let nonce = nonce else {
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit:

Suggested change
guard let nonce = nonce else {
guard let nonce else {

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 22c6ef0.

Networking/Networking/CookieNonce/CookieNonceAuthenticator.swift
import Alamofire
import Foundation

final class CookieNonceAuthenticator: RequestRetrier & RequestAdapter {
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be helpful to have some notes on the differences from the implementation in WordPressKit, so that it's easier to understand the changes since most of us aren't familiar with the cookie nonce auth 🙂

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added in 3be18f8 and 67ec443.

Networking/Networking/CookieNonce/CookieNonceAuthenticator.swift
let nonceRequest = try? URLRequest(url: nonceRetrievalURL, method: .get) else {
return invalidateLoginSequence(error: .invalidNewPostURL)
}
Task(priority: .medium) {
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ how is the priority determined? just thought the login request might be the top priority so that the user can log in.

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for networking requests, we want to use priorities lower than .userInitiated to avoid blocking the UI. .medium is my go-to for higher-priority networking requests - I hope that makes sense.

Networking/Networking/CookieNonce/CookieNonceAuthenticator.swift
}

func buildNonceRequestURL(base: URL) -> URL? {
URL(string: "admin-ajax.php?action=rest-nonce", relativeTo: base)
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ when I was comparing the implementation with the WordPressKit version, there was a check on version 5.3.0 (I assume it's the WP version) to use the ajax URL. just confirming that we don't need this check anymore?

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, we want to support only sites with WP from 5.6.0 to handle application passwords. This means only ajax nonce retrieval is needed. I updated the comment for the authenticator in 67ec443.

Networking/Networking/CookieNonce/CookieNonceAuthenticator.swift
var parameters = [URLQueryItem]()
parameters.append(URLQueryItem(name: "log", value: username))
parameters.append(URLQueryItem(name: "pwd", value: password))
parameters.append(URLQueryItem(name: "rememberme", value: "true"))
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ it looks like the request does not include a redirect_to URL anymore from the WPKit implementation, is it one of the major changes that fix the issue?

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the objective for this PR 😄.

Networking/Networking/CookieNonce/CookieNonceAuthenticator.swift Outdated
Comment on lines 160 to 163
guard !html.isEmpty else {
return nil
}
return html
Copy link
Contributor

@jaclync jaclync Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe can simplify?

Suggested change
guard !html.isEmpty else {
return nil
}
return html
html.isEmpty ? nil: html

@itsmeichigo
Copy link
Contributor Author

itsmeichigo commented Jan 18, 2023

I'm merging this PR to avoid blocking other PRs. @selanthiraiyan - if there's any issue with the renaming, I'll update it in a future PR.

@itsmeichigo itsmeichigo merged commit 9726ccb into trunk Jan 18, 2023
@itsmeichigo itsmeichigo deleted the try/cookie-nonce-authenticator-update branch January 18, 2023 04:36
@selanthiraiyan
Copy link
Contributor

selanthiraiyan commented Jan 18, 2023

Thanks for the ping @itsmeichigo !

I am afraid that adding ApplicationPassword prefix to the name wouldn't be right. Added my comment to the original thread here https://github.com/woocommerce/woocommerce-ios/pull/8659/files#r1073281562

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaclync jaclync jaclync approved these changes

@selanthiraiyan selanthiraiyan Awaiting requested review from selanthiraiyan

Assignees

@jaclync jaclync

Labels

feature: REST API Authenticating requests using application password and using REST API instead of Jetpack tunnel. type: enhancement A request for an enhancement.

Projects

None yet

Milestone

12.0

Development

Successfully merging this pull request may close these issues.

5 participants

@itsmeichigo @wpmobilebot @selanthiraiyan @jaclync